feat(server): improve configuration file handling - #423

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/server/config.h (1)

52-59: ⚠️ Potential issue | 🟠 Major

Preserve legacy flat config keys or migrate them explicitly.

Line 55 and Line 57 make project/rules the only deserialized config surface, while src/server/config.cpp still parses TOML/JSON directly into CliceConfig. Existing configs using top-level cache_dir, enable_indexing, worker counts, or compile_commands_paths can be silently ignored after upgrade unless a migration/alias layer is added.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/config.h` around lines 52 - 59, CliceConfig's deserializer
currently only exposes the nested fields project and rules which will drop
legacy top-level keys (e.g., cache_dir, enable_indexing, worker counts,
compile_commands_paths) parsed in src/server/config.cpp; update the
parsing/deserialization so it accepts legacy flat keys and maps them into the
new nested structure (or perform an explicit migration/aliasing pass) before
populating CliceConfig.project and CliceConfig.rules, leaving
annotation<std::vector<CompiledRule>, serde_schema::skip> compiled_rules
behavior unchanged; specifically modify the code that constructs CliceConfig in
src/server/config.cpp to detect legacy keys and copy them into the corresponding
ProjectConfig fields (and translate any worker/compile_commands settings to
their new locations) so existing configs are preserved.

src/server/master_server.cpp (1)

103-123: ⚠️ Potential issue | 🟠 Major

Make CDB auto-scan deterministic when multiple subdirectories match.

Lines 116-120 still pick the first compile_commands.json returned by directory_iterator, whose order is filesystem-dependent. Workspaces with build-debug/ and build-release/ can load different CDBs across machines or runs; collect matches, sort them, and warn when more than one candidate exists.

Proposed fix

-        if(!try_candidate(workspace_root)) {
+        if(!try_candidate(workspace_root)) {
+            std::vector<std::string> candidates;
             std::error_code ec;
             for(llvm::sys::fs::directory_iterator it(workspace_root, ec), end; it != end && !ec;
                 it.increment(ec)) {
                 if(it->type() == llvm::sys::fs::file_type::directory_file) {
-                    if(try_candidate(it->path()))
-                        break;
+                    auto candidate = path::join(it->path(), "compile_commands.json");
+                    if(llvm::sys::fs::exists(candidate))
+                        candidates.push_back(std::move(candidate));
                 }
             }
+            std::ranges::sort(candidates);
+            if(candidates.size() > 1)
+                LOG_WARN("Multiple compile_commands.json files found; using {}", candidates.front());
+            if(!candidates.empty())
+                cdb_path = std::move(candidates.front());
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/master_server.cpp` around lines 103 - 123, The current auto-scan
uses directory_iterator order to set cdb_path via the try_candidate lambda,
which is non-deterministic; change the logic to collect all matching
compile_commands.json paths (use try_candidate or inline check) into a
vector<string> (e.g., candidates), sort the vector lexicographically, then if
candidates is non-empty set cdb_path = candidates.front(); if candidates.size()
> 1 emit a warning listing the extra candidates (so users know which CDB was
chosen). Update the block that currently iterates workspace_root and uses
try_candidate to instead populate, sort, choose, and warn (referencing cdb_path,
try_candidate, workspace_root, and the directory_iterator loop to locate where
to change).

src/server/config.cpp (1)

80-85: ⚠️ Potential issue | 🟠 Major

Anchor relative project paths to workspace_root after substitution.

Line 84 still leaves configured compile_commands_paths relative to the server process CWD; the same applies to user-provided cache/index/logging paths on Lines 81-83. A config value like compile_commands_paths = ["build"] or cache_dir = ".clice" should resolve under the workspace, not wherever the server was launched.

Proposed fix

-    // Variable substitution on string fields.
-    substitute_workspace(p.cache_dir, workspace_root);
-    substitute_workspace(p.index_dir, workspace_root);
-    substitute_workspace(p.logging_dir, workspace_root);
-    for(auto& path: p.compile_commands_paths)
-        substitute_workspace(path, workspace_root);
+    // Variable substitution on string fields.
+    auto normalize_project_path = [&](std::string& value) {
+        substitute_workspace(value, workspace_root);
+        if(!value.empty() && !workspace_root.empty() && !llvm::sys::path::is_absolute(value))
+            value = path::join(workspace_root, value);
+    };
+
+    normalize_project_path(p.cache_dir);
+    normalize_project_path(p.index_dir);
+    normalize_project_path(p.logging_dir);
+    for(auto& path: p.compile_commands_paths)
+        normalize_project_path(path);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/config.cpp` around lines 80 - 85, The substituted paths
(p.cache_dir, p.index_dir, p.logging_dir and each entry in
p.compile_commands_paths) must be anchored to workspace_root when they are
relative; after calling substitute_workspace(...) detect if the resulting string
is a relative path and, if so, prepend/resolve it against workspace_root (e.g.
using std::filesystem::path(workspace_root) / path and assign back to the same
variable). Update the loop over p.compile_commands_paths and the three directory
assignments to normalize/absolute them relative to workspace_root when necessary
so config values like "build" or ".clice" resolve under the workspace rather
than the server CWD.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/server/master_server.cpp (2)

103-123: ⚠️ Potential issue | 🟠 Major

Make CDB auto-scan deterministic before choosing a match.

directory_iterator order is filesystem-dependent, so workspaces with multiple immediate build dirs can load different compile_commands.json files across machines/runs. Collect matching candidates, sort them, then pick a stable winner and optionally warn on ambiguity.

Suggested direction

-        if(!try_candidate(workspace_root)) {
+        if(!try_candidate(workspace_root)) {
+            std::vector<std::string> candidates;
             std::error_code ec;
             for(llvm::sys::fs::directory_iterator it(workspace_root, ec), end; it != end && !ec;
                 it.increment(ec)) {
                 if(it->type() == llvm::sys::fs::file_type::directory_file) {
-                    if(try_candidate(it->path()))
-                        break;
+                    auto candidate = path::join(it->path(), "compile_commands.json");
+                    if(llvm::sys::fs::exists(candidate))
+                        candidates.push_back(std::move(candidate));
                 }
             }
+            std::ranges::sort(candidates);
+            if(!candidates.empty()) {
+                if(candidates.size() > 1)
+                    LOG_WARN("Multiple compile_commands.json files found; using {}", candidates.front());
+                cdb_path = std::move(candidates.front());
+            }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/master_server.cpp` around lines 103 - 123, Auto-scan currently
picks the first compile_commands.json found via directory_iterator which is
non-deterministic; update the logic around try_candidate, cdb_path and the
directory iteration over workspace_root to collect all matching candidates into
a vector, sort them (e.g., lexicographically by path), then select a single
deterministic winner to assign to cdb_path and emit a warning if more than one
candidate was found to indicate ambiguity; ensure try_candidate is used only to
test paths and that the sorted selection replaces the existing early-exit
behavior so results are reproducible across machines/runs.

---

185-190: ⚠️ Potential issue | 🟠 Major

Do not let empty initializationOptions shadow clice.toml.

An empty JSON object is still a non-empty string, so {} will take the initializationOptions path, load defaults, and skip file-based config. Treat empty/unrelated initialization options as absent, or merge explicit JSON fields over the workspace config.

Suggested direction

         if(init.initialization_options.has_value()) {
             auto json =
                 kota::codec::json::to_json<kota::ipc::lsp_config>(*init.initialization_options);
-            if(json)
+            if(json && !is_empty_or_unrelated_config_json(*json))
                 init_options_json = std::move(*json);
         }

is_empty_or_unrelated_config_json should reject at least {} / null, and ideally only accept objects containing supported config keys such as project or rules.

Also applies to: 273-280

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/master_server.cpp` around lines 185 - 190, The current logic
accepts any non-null JSON (e.g. `{}`) from init.initialization_options and
causes init_options_json to override file-based clice.toml; update the handling
so empty or unrelated JSON is treated as absent: add/modify a helper (e.g.
is_empty_or_unrelated_config_json) that examines the result of
kota::codec::json::to_json<kota::ipc::lsp_config>(*init.initialization_options)
and returns false for `{}`, `null`, or objects lacking supported keys (like
"project" or "rules"); only move/assign to init_options_json when that helper
returns true. Apply the same check in both places where
init.initialization_options is parsed (the current block around
init_options_json and the similar block at lines 273-280) so file-based config
isn't shadowed by empty/unrelated JSON.

src/server/config.cpp (1)

84-85: ⚠️ Potential issue | 🟠 Major

Relative compile_commands_paths still resolved against CWD, not workspace root.

substitute_workspace only expands the ${workspace} placeholder; a plain relative entry like "build" is passed through unchanged and later resolved against the server process CWD in src/server/master_server.cpp. Configured CDB discovery in common setups (where users write compile_commands_paths = ["build"]) will fail to locate the file.

Proposed patch

-    for(auto& path: p.compile_commands_paths)
-        substitute_workspace(path, workspace_root);
+    for(auto& cdb_path: p.compile_commands_paths) {
+        substitute_workspace(cdb_path, workspace_root);
+        if(!workspace_root.empty() && !llvm::sys::path::is_absolute(cdb_path))
+            cdb_path = path::join(workspace_root, cdb_path);
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/config.cpp` around lines 84 - 85, substitute_workspace only
expands the ${workspace} token but leaves plain relative entries like "build"
unchanged; update the loop that processes p.compile_commands_paths to, after
calling substitute_workspace(path, workspace_root), detect if the resulting path
is not absolute and then resolve it by joining it with workspace_root (so
relative entries become workspace-root-relative instead of CWD-relative). Refer
to p.compile_commands_paths, substitute_workspace(path, workspace_root), and
workspace_root when implementing this fix.

src/server/config.cpp (1)

89-109: Include GlobPattern::create error detail in the log.

GlobPattern::create returns a descriptive std::string in std::unexpected (multiple /, brace expansion failures, sub-pattern errors), but the warning only prints the raw pattern string, leaving users to guess why the pattern was rejected.

Proposed diff

-            auto pat = GlobPattern::create(pattern_str);
-            if(!pat) {
-                LOG_WARN("Invalid glob pattern in rule: {}", pattern_str);
-                continue;
-            }
+            auto pat = GlobPattern::create(pattern_str);
+            if(!pat) {
+                LOG_WARN("Invalid glob pattern in rule: '{}' ({})", pattern_str, pat.error());
+                continue;
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/config.cpp` around lines 89 - 109, The warning for invalid glob
patterns should include the error message returned by GlobPattern::create
instead of only the raw pattern; update the branch where pat is falsy to extract
the descriptive error (from the returned unexpected/error payload of
GlobPattern::create) and pass it into LOG_WARN along with pattern_str so users
see why compilation failed (keep the existing check of
compiled.patterns.empty(), and adjust the invalid-pattern log in the loop that
handles pat failures inside the rule-processing loop).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/unit/server/config_tests.cpp (1)

315-321: WorkspaceSubstRepeated is pinning a surprising behavior — consider asserting post-normalization instead.

Expected value "/root/a//root/b" contains a // that comes purely from naive string replacement (user-written /${workspace} + workspace_root = "/root"). Any future PR that normalizes output (e.g., collapsing duplicate separators, which llvm::sys::path helpers sometimes do) would break this test even though it would be an improvement. If the point is "every occurrence is substituted" rather than "no normalization happens", consider asserting both halves independently or counting occurrences, so the test doesn't pin a debatable artifact.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/server/config_tests.cpp` around lines 315 - 321, The test
WorkspaceSubstRepeated pins an implementation artifact by expecting
"/root/a//root/b" after CliceConfig::apply_defaults substitutes
project.cache_dir; instead update the test to assert the semantic intent (that
every "${workspace}" occurrence is substituted) rather than exact raw string
with duplicate separators—for example, verify that project.cache_dir contains
two occurrences of "/root" in the expected positions or assert the two path
segments independently after normalization; locate the TEST_CASE
WorkspaceSubstRepeated and CliceConfig.project.cache_dir and change the
EXPECT_EQ to an assertion that checks occurrence count or compares normalized
segments so future path-normalization changes won't break the test.

src/server/config.cpp (2)

46-55: XDG cache key is workspace_root as-received — not normalized.

xxh3_64bits(workspace_root) is computed on the raw string passed by the caller, so logically-equivalent roots (/home/user/proj, /home/user/proj/, /home/user/./proj, a path with a different case on case-insensitive FS, a symlinked path) map to distinct cache dirs. That means the same project opened two slightly different ways on the same machine will cold-start the index twice and consume extra disk. Consider canonicalizing via llvm::sys::fs::real_path/remove_dots (with a safe fallback when the path doesn't yet exist) before hashing. Low-severity but worth doing while the layout is fresh.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/config.cpp` around lines 46 - 55, The cache key is computed from
the raw workspace_root string (xxh3_64bits(workspace_root)), causing equivalent
paths to map to different directories; normalize the path before hashing by
resolving dots/symlinks and canonicalizing where possible (use
llvm::sys::fs::real_path or remove_dots) and fall back to a safe normalization
when the path does not exist (e.g., remove_dots or weakly canonicalize and
normalize case on case-insensitive platforms) so that workspace_root is
normalized prior to calling xxh3_64bits; update the code around workspace_root,
xxh3_64bits, and the path::join creation so the hash is computed from the
normalized_path and keep existing create_directories logic unchanged.

---

188-195: Log string split is a formatter trap.

Adjacent string-literal concatenation between a trailing-space literal and the format string works, but it means the format string is assembled at compile time across two literals — easy to silently lose a {}-count when someone edits only one half. Since this already logs worker_memory_limit that isn't part of what ends up happening (e.g., a user-overridden memory_limit), consider just writing a single contiguous format string.

Nit

-    LOG_INFO(
-        "No clice.toml found, using default configuration " "(stateful={}, stateless={}, memory_limit={}MB)",
-        config.project.stateful_worker_count.value,
-        config.project.stateless_worker_count.value,
-        config.project.worker_memory_limit.value / (1024 * 1024));
+    LOG_INFO("No clice.toml found, using default configuration "
+             "(stateful={}, stateless={}, memory_limit={}MB)",
+             config.project.stateful_worker_count.value,
+             config.project.stateless_worker_count.value,
+             config.project.worker_memory_limit.value / (1024 * 1024));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/config.cpp` around lines 188 - 195, The log message in the
fallback branch uses adjacent string-literal concatenation which risks
mismatched format placeholders; update the LOG_INFO call in this block (the call
that follows CliceConfig config; config.apply_defaults(workspace_root);) to use
a single contiguous format string that includes all three placeholders in the
correct order for config.project.stateful_worker_count.value,
config.project.stateless_worker_count.value, and
config.project.worker_memory_limit.value / (1024 * 1024), ensuring spacing and
braces are correct so the formatter and arguments cannot get out of sync.

src/server/config.h (1)

33-39: Intentional split between defaulted<> and std::optional<> is fine, just worth calling out.

enable_indexing/idle_timeout_ms use std::optional specifically so apply_defaults can distinguish "unset" from an explicit false/0 (see ApplyDefaultsPreserveSet). The rest use defaulted<> because their zero/empty sentinel is also a valid "unset" signal. Consider a brief comment on these two lines so future contributors don't "normalize" them to defaulted<> and silently break user-overrides of false.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/config.h` around lines 33 - 39, Add a brief clarifying comment
above the two members so future maintainers know why enable_indexing and
idle_timeout_ms use std::optional instead of kota::meta::defaulted: explain that
ApplyDefaultsPreserveSet needs to distinguish "unset" from explicit values
(false/0) so enable_indexing and idle_timeout_ms must be optional, whereas
stateful_worker_count/stateless_worker_count/worker_memory_limit use defaulted<>
because their zero/empty sentinel can represent "unset". Reference the member
names enable_indexing, idle_timeout_ms and the function/behavior
ApplyDefaultsPreserveSet in the comment.